Share commentPrefix's rule in one place in LibCodeGenSlow - #107
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes #73
What
bytes(comment).length == 0 ? "\n" : string.concat("\n", comment, "\n")waspasted into all four
…ConstantStringSlowbuilders intest/lib/LibCodeGenSlow.sol. That isLibCodeGen.commentPrefix's bodycharacter for character, in four places. The four copies now call one
commentPrefixSlow.Keeping the reference independently derived
LibCodeGenSlowis the differential reference forLibCodeGen. Its value isthat it derives the expected output instead of restating the implementation, so
a shared design error shows up as a disagreement rather than moving both sides
at once. Lifting the pasted expression verbatim into a shared function would
have turned four restatements into one restatement — the same defect, better
factored — so that is not what this does.
commentPrefixSlowderives the prefix from the rule as stated in prose ratherthan from the code that implements it. The rule is "a declaration is preceded by
one blank line always, and by a comment line only when there is a comment", so
the reference lists those lines and joins them, terminating each:
LibCodeGen.commentPrefixchooses between two whole prefix literals. Thereference spells out neither of those literals, and states the line count and
the per-line text as separate facts. Neither side can be updated by pasting from
the other, which is the property the file exists to have.
lines[0] = ""is redundant in the sense that a freshstring[]element isalready the empty string. It is kept because the blank line is half the rule and
the array is where the rule is stated. M4 below breaks that line and the suite
reds, so it is load bearing for the tests as written, not decoration.
The library's
@noticedisclosed the copy as the one place the two sides statethe same rule. That is no longer what the file does, so the paragraph now
describes the line assembly.
Where this went past the issue's proposed fix
The issue asked to extract
commentPrefixSlowand keep the docstring'sdisclosure attached to it. Extracting alone would have left a verbatim
restatement, so the body was re-derived as above, and the disclosure was
replaced rather than moved — there is no longer a restatement to disclose. The
docstring on
commentPrefixSlowsays how its derivation differs from thelibrary's instead.
Tests
test/lib/LibCodeGen.commentPrefix.t.solis new, next to its siblings intest/lib/. Two literal assertions pincommentPrefixSlow's own two cases sothe reference is not held up only by agreeing with the implementation, and a
fuzz asserts
LibCodeGen.commentPrefix(comment) == LibCodeGenSlow.commentPrefixSlow(comment)over 2048 comments. The fourbuilders' existing differential fuzzes cover the extraction not changing what
they emit.
QA
testCommentPrefixSlowEmptyComment,testCommentPrefixSlowWithComment,testCommentPrefixMatchesSlow- eachfails on base, verified by running
nix develop -c forge test --match-path test/lib/LibCodeGen.commentPrefix.t.solon the branch with the test written and
commentPrefixSlownot yet added:the compile fails with
Error (9582): Member "commentPrefixSlow" not found(full output below).
and killing tests in the table below.
? 1 : 2->? 2 : 2->testCommentPrefixSlowEmptyComment;lines[1] = comment->lines[1] = ""->
testCommentPrefixSlowWithComment; drop the"\n"terminator in the join-> both literal pins;
lines[0] = ""->lines[0] = "X"-> both literalpins; loop
i = 0->i = 1-> both literal pins; and three mutants ofLibCodeGen.commentPrefixitself (? "\n"->? "", drop the trailing"\n",== 0->!= 0) each killed throughtestCommentPrefixMatchesSlow.LibCodeGen.commentPrefix's docstringand in the
forge fmtbehaviour it exists for - one blank line always, acomment line only when there is a comment. The two literal assertions carry
the expected strings written out, and
commentPrefixSlowbuilds from thelist of lines rather than from the implementation's expression, so neither
the reference nor the tests take their expectation from
LibCodeGen.commentPrefix's body atLibCodeGenSlow.sol:72,84,96,110- covered, allfour now call
commentPrefixSlow,grepfinds no remaining copy of theexpression in the file. The issue's own proposed fix (extract and move the
docstring disclosure) is covered and extended: the body is re-derived rather
than lifted, so there is no restatement left to disclose.
Red first. With the test written and
commentPrefixSlownot yet added,nix develop -c forge test --match-path test/lib/LibCodeGen.commentPrefix.t.sol:The finding is a code-quality one — four copies of a rule that already agreed
with each other — so there is no behavioural red to show. The red is that the
shared function and the assertions that hold it did not exist. The mutation
matrix below is what demonstrates the new code is covered rather than merely
present.
Green after.
nix develop -c forge test:Ran 17 test suites: 137 tests passed, 0 failed, 0 skipped (137 total tests)— 134 before this branch, plus the three new ones.
nix develop -c forge fmt --checkexits 0, no files listed.Mutation matrix. Each mutant: applied by
sed, the edit proved to havelanded by a non-empty
git diffbefore the suite ran (a pattern that matchednothing aborts as INVALID rather than reading as "survived"), whole suite run,
git checkoutto restore. Control run first: 137 passed / 0 failed. Everymutant is killed; the counts are out of the same 137.
LibCodeGenSlow? 1 : 2→? 2 : 2(comment line emitted for an empty comment)testCommentPrefixSlowEmptyComment,testCommentPrefixMatchesSlowLibCodeGenSlowlines[1] = comment→lines[1] = ""testCommentPrefixSlowWithComment,testCommentPrefixMatchesSlowLibCodeGenSlow"\n"line terminator in the joinLibCodeGenSlowlines[0] = ""→lines[0] = "X"(blank first line)LibCodeGenSlowi = 0→i = 1(skip the blank line)LibCodeGen? "\n"→? ""(no blank line when no comment)testCommentPrefixMatchesSlowLibCodeGen"\n"after the commenttestCommentPrefixMatchesSlowLibCodeGen== 0→!= 0testCommentPrefixMatchesSlowM1–M5 mutate the new reference and are killed by the new tests and by the four
builders' differentials. M6–M8 mutate the implementation the reference guards
and are killed through the differential, so the extraction did not cost the
suite any of the discrimination the four copies had.